Immutable CRAIEntry #1256

jmthibault79 · 2019-01-09T15:55:35Z

Description

Code cleanup for CRAIEntry

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

- explicitly set nOfRecords and index to 0 - remove unused sliceIndex

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

jmthibault79 · 2019-01-09T16:01:36Z

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

-        }
-        return entries;
+        return Arrays.stream(container.slices)
+                .map(slice -> slice.getCRAIEntry(slice.containerOffset))


Can't necessarily derive the container offset from the Slice because sometimes it conflicts with the Container's own offset - will need to look into why.

I don't have high confidence that these are managed correctly by the existing code, but in this case it's using the value from the slice anyway. Can we add a method to slice that uses it's offset directly, and if there is some other context for which that fails, maybe also add a static with this signature for that one case until we figure out whats going on, since thats the broken one ?

BTW, is slice.containerOffset supposed to be the offset of the containing container, or is it the offset of the slice relative to the beginning of the containing container ? Might be good to document those.

Also, any reason not to move this method to container getCRAIEntries just as it is, and have it return the list of entries. Eventually it should end up there I think, and would be similar to the new one you added to Slice.

OK, these suggestions make sense.

Slice.containerOffset is the byte offset of the Container from the beginning of the stream.

Slice.offset is the byte offset of the Slice from the beginning of its Container.

I'll add comments making this more clear to Slice

jmthibault79 · 2019-01-09T16:02:39Z

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

@@ -114,19 +111,6 @@ public int compareTo(final CRAIEntry o) {
        return (int) (containerStartOffset - o.containerStartOffset);
    }

-    @Override
-    public CRAIEntry clone() throws CloneNotSupportedException {


only tests were using this

jmthibault79 · 2019-01-09T16:03:34Z

src/main/java/htsjdk/samtools/cram/CRAIIndex.java

-                    e.sliceIndex = i;
-
-                    entries.add(e);
+                    final Map<Integer, AlignmentSpan> spans = s.getMultiRefAlignmentSpans(container.header, ValidationStringency.DEFAULT_STRINGENCY);


equivalent to what getCRAIEntriesForMultiRefSlice() did

jmthibault79 · 2019-01-09T16:05:36Z

src/test/java/htsjdk/samtools/SamIndexesTest.java

-        entry.sliceOffset = 3;
-        entry.sliceSize = 4;
-        entry.containerStartOffset = 5;
+        final CRAIEntry entry = CRAIEntryTest.newEntry(0, 1, 2, 5, 3, 4);


preserves the values set above rather than the order

cmnbroad

A few questions/suggestions for the initial pass.

cmnbroad · 2019-01-09T19:57:47Z

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

-        }
-        return entries;
+        return Arrays.stream(container.slices)
+                .map(slice -> slice.getCRAIEntry(slice.containerOffset))


I don't have high confidence that these are managed correctly by the existing code, but in this case it's using the value from the slice anyway. Can we add a method to slice that uses it's offset directly, and if there is some other context for which that fails, maybe also add a static with this signature for that one case until we figure out whats going on, since thats the broken one ?

BTW, is slice.containerOffset supposed to be the offset of the containing container, or is it the offset of the slice relative to the beginning of the containing container ? Might be good to document those.

src/main/java/htsjdk/samtools/cram/CRAIIndex.java

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

src/main/java/htsjdk/samtools/cram/structure/Slice.java

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

cmnbroad · 2019-01-09T21:21:41Z

src/main/java/htsjdk/samtools/cram/CRAIEntry.java

-        }
-        return entries;
+        return Arrays.stream(container.slices)
+                .map(slice -> slice.getCRAIEntry(slice.containerOffset))


Also, any reason not to move this method to container getCRAIEntries just as it is, and have it return the list of entries. Eventually it should end up there I think, and would be similar to the new one you added to Slice.

src/main/java/htsjdk/samtools/cram/CRAIIndex.java

codecov-io · 2019-01-09T22:24:04Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5a0ab68). Click here to learn what that means.
The diff coverage is 85.714%.

@@            Coverage Diff             @@
##             master     #1256   +/-   ##
==========================================
  Coverage          ?   69.359%           
  Complexity        ?      8164           
==========================================
  Files             ?       548           
  Lines             ?     32682           
  Branches          ?      5521           
==========================================
  Hits              ?     22668           
  Misses            ?      7795           
  Partials          ?      2219

Impacted Files	Coverage Δ	Complexity Δ
...java/htsjdk/samtools/cram/structure/Container.java	`84.211% <100%> (ø)`	`8 <1> (?)`
...ain/java/htsjdk/samtools/cram/structure/Slice.java	`45.536% <100%> (ø)`	`22 <2> (?)`
src/main/java/htsjdk/samtools/cram/CRAIIndex.java	`84.615% <81.818%> (ø)`	`27 <4> (?)`
src/main/java/htsjdk/samtools/cram/CRAIEntry.java	`71.053% <86.364%> (ø)`	`17 <8> (?)`

cmnbroad

@lbergelson LGTM as an incremental step.

lbergelson · 2019-01-10T21:06:37Z

@cmnbroad @jmthibault79 👍 Thank you.

…ry that affects CramSource in Disq. See the note about incompatible CRAM changes at https://github.com/samtools/htsjdk/releases/tag/2.17.0. This commit uses the new CRAIEntry API introduced in samtools/htsjdk#1256

…ry that affects CramSource in Disq. See the note about incompatible CRAM changes at https://github.com/samtools/htsjdk/releases/tag/2.17.0. This commit uses the new CRAIEntry API introduced in samtools/htsjdk#1256 remove use of SAMRecord.getIndexBin() since the method has been removed Convert catch for EOFException -> RuntimeEOFException Use htsjdk 2.19.0-rc3-SNAPSHOT Use htsjdk 2.19.0

* The changes to CRAM have introduced an incompatible change to CRAIEntry that affects CramSource in Disq. See the note about incompatible CRAM changes at https://github.com/samtools/htsjdk/releases/tag/2.17.0. This commit uses the new CRAIEntry API introduced in samtools/htsjdk#1256 * Convert catch for EOFException -> RuntimeEOFException * Use htsjdk 2.19.0

encapsulate CRAIEntry fields

06070a0

- explicitly set nOfRecords and index to 0 - remove unused sliceIndex

jmthibault79 added the cram label Jan 9, 2019

jmthibault79 commented Jan 9, 2019

View reviewed changes

src/main/java/htsjdk/samtools/cram/CRAIEntry.java Show resolved Hide resolved

containerStartByteOffset

448548c

jmthibault79 commented Jan 9, 2019

View reviewed changes

slightly better test

3774387

jmthibault79 requested review from cmnbroad and lbergelson January 9, 2019 16:09

cmnbroad requested changes Jan 9, 2019

View reviewed changes

jmthibault79 added 3 commits January 9, 2019 17:17

comments about Slice indexing values

bbd533f

Add getCraiEntry() and getCraiEntries()

91ced43

all caps CRAI

5ed180f

offset comments in CRAIEntry

8e4098c

cmnbroad approved these changes Jan 10, 2019

View reviewed changes

jmthibault79 merged commit 94f0967 into master Jan 10, 2019

jmthibault79 deleted the jt_CRAIEntry branch January 10, 2019 21:26

tomwhite mentioned this pull request Jan 16, 2019

Upgrade to htsjdk 2.19.0 disq-bio/disq#77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable CRAIEntry #1256

Immutable CRAIEntry #1256

jmthibault79 commented Jan 9, 2019 •

edited

Loading

jmthibault79 Jan 9, 2019 •

edited

Loading

cmnbroad Jan 9, 2019

cmnbroad Jan 9, 2019

jmthibault79 Jan 9, 2019

jmthibault79 Jan 9, 2019

jmthibault79 Jan 9, 2019

jmthibault79 Jan 9, 2019

cmnbroad left a comment

cmnbroad Jan 9, 2019

cmnbroad Jan 9, 2019

codecov-io commented Jan 9, 2019 •

edited

Loading

cmnbroad left a comment

lbergelson commented Jan 10, 2019

Immutable CRAIEntry #1256

Immutable CRAIEntry #1256

Conversation

jmthibault79 commented Jan 9, 2019 • edited Loading

Description

Checklist

jmthibault79 Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 9, 2019 • edited Loading

Codecov Report

cmnbroad left a comment

Choose a reason for hiding this comment

lbergelson commented Jan 10, 2019

jmthibault79 commented Jan 9, 2019 •

edited

Loading

jmthibault79 Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading